Skip to content

Services: Stdout/stderr and various error case handling - #483

Merged
aomarks merged 11 commits into
servicefrom
service.8
Oct 26, 2022
Merged

Services: Stdout/stderr and various error case handling#483
aomarks merged 11 commits into
servicefrom
service.8

Conversation

@aomarks

@aomarks aomarks commented Oct 26, 2022

Copy link
Copy Markdown
Member

More incremental progress on services:

  • Show service stdout/stderr
  • Make services wait for their own services to start
  • Make standard scripts and services fail when a service exits unexpectedly (both while they are running and before)
  • Wait for services to shut down before ending an execution, including a refactor to make this simpler

Also:

  • Gracefully close IPC socket in our test processes on SIGINT. Fixes occasional ECONNRESET errors.
  • Make our test stdout/stderr matcher only consume up to the match, instead of also consuming everything beyond.
  • Minor renaming

Part of #33

@aomarks
aomarks requested a review from justinfagnani October 26, 2022 00:02
Comment thread src/test/util/test-rig.ts
stdoutLastIndex = Math.max(stdoutLastIndex, re.lastIndex);
} else if (re.test(this._stderr)) {
stderrLastIndex = Math.max(stderrLastIndex, re.lastIndex);
deferred.resolve();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make the order of these two lines match above?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread src/test/util/test-rig.ts Outdated
for (const {re, deferred} of this._logMatchers) {
if (re.test(this._stdout)) {
deferred.resolve();
stdoutLastIndex = Math.max(stdoutLastIndex, re.lastIndex);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you ever reset re.lastIndex? Since the RE's with /g or /y flags are stateful, you can get bad behavior if you reuse a regex. Or will you recommend not using those flags in docs?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm yeah. Actually using lastIndex was completely wrong anyway, because it's only set when the g or s flags are on.

Switched to using exec (so that we always get one match even if the g flag was used), and checking the index + length, instead of lastIndex.

I also noticed we weren't removing matchers after they matched. We don't care about lastIndex now because it can only match once.

Comment thread src/executor.ts
const result = await this.getExecution(this._rootConfig).execute();
// Wait for services to shut down.
// TODO(aomarks) In watch mode, directly-invoked scripts (and the services
// they depend on) should not block here, since they should continue

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this always the case? Should some services restart in watch mode depending onhow they use their dependencies and files?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In watch mode, the directly invoked services will get passed off to the next iteration's execution, which will decide whether to shut down or preserve the child processes.

@aomarks
aomarks merged commit 81af6b3 into service Oct 26, 2022
@aomarks
aomarks deleted the service.8 branch October 26, 2022 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants